Skip to content

fix(core): validate replay operation identity - #698

Merged
zhongkechen merged 13 commits into
mainfrom
fix/replay-operation-identity
Sep 3, 2026
Merged

fix(core): validate replay operation identity#698
zhongkechen merged 13 commits into
mainfrom
fix/replay-operation-identity

Conversation

@zhongkechen

Copy link
Copy Markdown
Contributor

Summary

  • validate checkpoint type, subtype, and name before operation-specific replay handling
  • fail mismatches with NonDeterministicExecutionError instead of consuming another operation's checkpoint
  • cover type, subtype, and name drift through unit and end-to-end regression tests

Testing

  • 1,587 core non-e2e tests passed
  • 46 core e2e tests passed
  • hatch run dev-core:typecheck
  • hatch fmt --check

Closes #692

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 31, 2026 18:58 — with GitHub Actions Inactive
@zhongkechen
zhongkechen had a problem deploying to ai-pr-review-runtime August 31, 2026 19:11 — with GitHub Actions Failure
@zhongkechen
zhongkechen had a problem deploying to ai-pr-review-runtime August 31, 2026 19:11 — with GitHub Actions Failure
@zhongkechen
zhongkechen had a problem deploying to ai-pr-review-runtime August 31, 2026 19:31 — with GitHub Actions Failure
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 31, 2026 19:31 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen had a problem deploying to ai-pr-review-runtime August 31, 2026 22:50 — with GitHub Actions Failure
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 31, 2026 22:50 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

Found two replay-determinism gaps involving virtual and FLAT/NESTED child contexts.

Reviewed commit be1d349f153bf74d61e1104745b9ae77513f0697. Workflow run

@yaythomas

Copy link
Copy Markdown
Contributor

I behavior-tested this branch against the two JS-side fixes for the same defect family (aws-durable-execution-sdk-js#867 and aws-durable-execution-sdk-js#894, and this change works perfectly!

the divergence test below returns SUCCEEDED (silent checkpoint substitution, #692) on main and flips to FAILED carrying a typed NonDeterministicExecutionError with mismatch detail on this branch, including preserving the error type into the ErrorObject, which the JS side had to fix separately in #867.

Offering two example-level test files as complementary regression coverage (parity with the test matrices those JS PRs added). All five pass on this branch; the divergence case is red on main, so it directly witnesses this PR's fix. Maybe can add?

test/map/test_map_summarized_replay_matrix.py — parity with JS #894's FLAT/NESTED/small-result matrix:

"""Tests for map summarized replay across a suspension — parity with JS PR #894.

Matrix (mirrors packages/aws-durable-execution-sdk-js-examples map/flat-summarized-replay):
- FLAT nesting, >256KB result  -> ReplayChildren rebuild across a suspension (the bug path)
- NESTED nesting, >256KB result -> control: context checkpoints exist, rebuild trivially correct
- FLAT nesting, small result    -> control: stays within one checkpoint, no rebuild at all
"""

import pytest
from aws_durable_execution_sdk_python.config import MapConfig, NestingType
from aws_durable_execution_sdk_python.context import DurableContext
from aws_durable_execution_sdk_python.execution import (
    InvocationStatus,
    durable_execution,
)
from aws_durable_execution_sdk_python.waits import Duration

from test.conftest import deserialize_operation_payload

ITEM_COUNT = 8
LARGE_ITEM_PAYLOAD_BYTES = 40 * 1024  # 8 x 40KB ≈ 320KB > 256KB limit
SMALL_ITEM_PAYLOAD_BYTES = 100  # 8 x 100B — far below the limit


def make_handler(nesting: NestingType, payload_bytes: int):
    @durable_execution
    def handler(_event, context: DurableContext):
        items = list(range(ITEM_COUNT))

        batch = context.map(
            inputs=items,
            func=lambda ctx, item, index, _: ctx.step(
                lambda _s: {"item": item, "payload": "x" * payload_bytes},
                name=f"resolve-{index}",
            ),
            name="resolve-pages",
            config=MapConfig(nesting_type=nesting),
        )

        live_result_count = context.step(
            lambda _: len(batch.get_results()), name="record-live-count"
        )

        context.wait(Duration.from_seconds(86400), name="suspend")

        replayed_result_count = context.step(
            lambda _: len(batch.get_results()), name="record-replayed-count"
        )
        replayed_items = sorted(r["item"] for r in batch.get_results())

        return {
            "live_result_count": live_result_count,
            "replayed_result_count": replayed_result_count,
            "replayed_items": replayed_items,
        }

    return handler


HANDLERS = {
    "flat-large": make_handler(NestingType.FLAT, LARGE_ITEM_PAYLOAD_BYTES),
    "nested-large": make_handler(NestingType.NESTED, LARGE_ITEM_PAYLOAD_BYTES),
    "flat-small": make_handler(NestingType.FLAT, SMALL_ITEM_PAYLOAD_BYTES),
}


@pytest.mark.example
@pytest.mark.parametrize("case", ["flat-large", "nested-large", "flat-small"])
def test_map_summarized_replay_matrix(case):
    """The batch reconstructed after suspend/resume must equal the live batch."""
    from aws_durable_execution_sdk_python_testing.runner import (
        DurableFunctionTestRunner,
    )

    with DurableFunctionTestRunner(handler=HANDLERS[case]) as runner:
        result = runner.run(input='"test"', execution_timeout=30)

    assert result.status is InvocationStatus.SUCCEEDED, (
        f"[{case}] expected SUCCEEDED, got {result.status}: {result.error}"
    )
    output = deserialize_operation_payload(result.result)
    assert output["live_result_count"] == ITEM_COUNT, f"[{case}] live count wrong"
    assert output["replayed_result_count"] == ITEM_COUNT, (
        f"[{case}] replayed count {output['replayed_result_count']} != {ITEM_COUNT} — "
        "summarized-replay rebuild lost terminal items"
    )
    assert output["replayed_items"] == list(range(ITEM_COUNT)), (
        f"[{case}] replayed items {output['replayed_items']}"
    )

test/map/test_replay_validation_parity.py — parity with JS #867's replay-validation composed tests (divergence → FAILED with typed diagnostic + stable-replay control):

"""Non-deterministic replay fails closed with diagnostic — parity with JS PR #867.

Two cases, mirroring replay-validation.composed.test.ts:
1. Divergent replay (STEP checkpointed, CHAINED_INVOKE replayed at the same
   position) -> FAILED, error type preserved as NonDeterministicExecutionError,
   never PENDING.
2. Stable control (identical replay) -> SUCCEEDED.
"""

import pytest
from aws_durable_execution_sdk_python.context import DurableContext
from aws_durable_execution_sdk_python.execution import (
    InvocationStatus,
    durable_execution,
)
from aws_durable_execution_sdk_python.waits import Duration
from aws_durable_execution_sdk_python_testing.runner import DurableFunctionTestRunner

# Deliberately module-global: DurableFunctionTestRunner replays in the SAME
# process, so this flag survives across the suspend/resume boundary — which is
# exactly what makes the divergence reachable. First invocation takes the STEP
# branch; the replay invocation (same process, flag already set) takes the
# CHAINED_INVOKE branch at the same logical position. Against the real service
# the equivalent divergence arises from any replay-visible state change
# (e.g. control flow derived from a corrupted batch rebuild, see JS PR #894).
_STATE = {"diverged_once": False}


@durable_execution
def divergent_handler(_event, context: DurableContext):
    context.step(lambda _: "anchor", name="anchor")
    if not _STATE["diverged_once"]:
        _STATE["diverged_once"] = True
        context.step(lambda _: "first-run-step", name="divergent")
        context.wait(Duration.from_seconds(86400), name="suspend")
    else:
        context.invoke(function_name="some-worker", payload={"x": 1}, name="divergent")
    return {"reached_end": True}


@durable_execution
def stable_handler(_event, context: DurableContext):
    context.step(lambda _: "anchor", name="anchor")
    context.step(lambda _: "stable-step", name="stable")
    context.wait(Duration.from_seconds(86400), name="suspend")
    context.step(lambda _: "post", name="post")
    return {"reached_end": True}


@pytest.mark.example
def test_divergent_replay_fails_closed_with_diagnostic():
    """Type mismatch on replay -> FAILED carrying NonDeterministicExecutionError."""
    _STATE["diverged_once"] = False
    with DurableFunctionTestRunner(handler=divergent_handler) as runner:
        result = runner.run(input='"test"', execution_timeout=30)

    # Never PENDING for a permanent fault (the #867 misclassification).
    assert result.status is not InvocationStatus.PENDING
    assert result.status is InvocationStatus.FAILED, (
        f"Expected FAILED, got {result.status}"
    )
    # Diagnostic must reach the caller with the error type preserved
    # (JS #867 fixed exactly this: bare Error instead of the typed error).
    assert result.error is not None, "FAILED result carries no error object"
    assert "NonDeterministicExecutionError" in (result.error.type or ""), (
        f"Error type not preserved: {result.error.type}"
    )
    assert "type checkpoint='STEP'" in (result.error.message or ""), (
        f"Diagnostic lacks mismatch detail: {result.error.message}"
    )


@pytest.mark.example
def test_stable_replay_control_succeeds():
    """Identical replay across a suspension must succeed (control case)."""
    with DurableFunctionTestRunner(handler=stable_handler) as runner:
        result = runner.run(input='"test"', execution_timeout=30)
    assert result.status is InvocationStatus.SUCCEEDED, (
        f"Control case failed: {result.status}: {result.error}"
    )

Verification on this branch (a2233e9):

test/map/test_map_summarized_replay_matrix.py::test_map_summarized_replay_matrix[flat-large] PASSED
test/map/test_map_summarized_replay_matrix.py::test_map_summarized_replay_matrix[nested-large] PASSED
test/map/test_map_summarized_replay_matrix.py::test_map_summarized_replay_matrix[flat-small] PASSED
test/map/test_replay_validation_parity.py::test_divergent_replay_fails_closed_with_diagnostic PASSED
test/map/test_replay_validation_parity.py::test_stable_replay_control_succeeds PASSED

Same five on main: the divergence case fails with SUCCEEDED (the #692 silent substitution), the other four pass — confirming this PR is the behavior change under test.

@zhongkechen
zhongkechen dismissed yaythomas’s stale review September 3, 2026 19:20

Both concerns were addressed in 687928a: the incomplete inference and O(started × operations) scan were removed, and regressions verify ambiguous histories preserve STARTED with zero operation-map reads. Requesting a fresh review of the updated PR.

@yaythomas yaythomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beauitful, thank you! 🥇

@zhongkechen
zhongkechen merged commit 38d94dc into main Sep 3, 2026
54 of 56 checks passed
@zhongkechen
zhongkechen deleted the fix/replay-operation-identity branch September 3, 2026 21:26
yaythomas added a commit that referenced this pull request Sep 3, 2026
- Rename to migration-1.x-to-2.0.md; name the release 2.0, not 2.x
- Add a Why upgrade to 2.0 section listing only 2.0 additions
- Rewrite in active voice; remove filler and passive constructions
- Rename batching heading to Removed 1.x-only names (covers ChainedInvoke enums)
- Add max_concurrency in-flight semantics section
- Add replay operation identity validation (#698)
- Add wait_for_condition unreadable-polling-state failure (was silent
  restart from initial_state in 1.x)
- Scope RetryableSerDesError guidance to invocation replay and step
  semantics; scope exhaustion behavior to create_wait_strategy
- Correct the callbacks note: create_callback().result() still exists
- Move the experimental plugin note to its own section with a doc link
- Mark the grep checklist as non-exhaustive
yaythomas added a commit that referenced this pull request Sep 3, 2026
- Rename to migration-1.x-to-2.0.md; name the release 2.0, not 2.x
- Add a Why upgrade to 2.0 section listing only 2.0 additions
- Rewrite in active voice; remove filler and passive constructions
- Rename batching heading to Removed 1.x-only names
- Add max_concurrency in-flight semantics section
- Add replay operation identity validation (#698)
- Add wait_for_condition unreadable-polling-state failure
- Scope RetryableSerDesError guidance to invocation replay and
  step semantics; scope exhaustion to create_wait_strategy
- Correct callbacks note: create_callback().result() still exists
- Move the experimental plugin note to its own linked section
- Mark the grep checklist as non-exhaustive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Replay mismatch silently consumes a checkpoint from a different operation

2 participants